Skip to content

C++: Positively phrased sanitizer in cpp/non-constant-format#12003

Merged
MathiasVP merged 1 commit intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
MathiasVP:positive-formulated-sanitizer-nonconst-format
Jan 27, 2023
Merged

C++: Positively phrased sanitizer in cpp/non-constant-format#12003
MathiasVP merged 1 commit intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
MathiasVP:positive-formulated-sanitizer-nonconst-format

Conversation

@MathiasVP
Copy link
Copy Markdown
Contributor

The sanitizer in the cpp/non-constant-format query was very fragile: it's supposed to look at the type of the node, and mark it as a sanitizer if the type reveals that the node could not possible hold a string.

This was easy to do before we introduced indirect dataflow nodes: we could just mark all non-pointer nodes as sanitizers because we were tracking a value of type char*. However, when we track indirect nodes as well, those nodes can have type char (since we're tracking the indirection of the char).

So when we introduced indirect nodes, we modified the sanitizer to not sanitize indirect nodes (for instance, by saying that node.asIndirectExpr() shouldn't exist). However, this relies on all indirect nodes having a result for asIndirectExpr() which doesn't have to be true (since plenty of dataflow nodes do not map to an expression).

Instead, this PR rephrases the sanitizer to only sanitize nodes for which node.asExpr() holds. This predicate never has a result for indirect nodes, so the effect should be the same, but it won't depend on each indirect dataflow node having a result for asIndirectExpr().

…o longer incorrectly sanitize indirect nodes for which there is no result for 'asIndirectExpr'.
@MathiasVP MathiasVP added the C++ label Jan 27, 2023
@MathiasVP MathiasVP requested a review from a team as a code owner January 27, 2023 10:15
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Jan 27, 2023
Copy link
Copy Markdown
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if DCA is happy.

@MathiasVP
Copy link
Copy Markdown
Contributor Author

DCA showed nothing exciting (🎉 I guess!)

@MathiasVP MathiasVP merged commit e48c93a into github:mathiasvp/replace-ast-with-ir-use-usedataflow Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants